AI Automated Review

Analysis engine: MiniMax@https://litellm.jory.dev/v1 (anthropic)

Recommendation: Approve

This PR delivers a focused, well-tested data layer for the early-game milestone goal system described in #132. The implementation cleanly separates evaluation logic from persistence and UI concerns, matches existing repo patterns (e.g. scripts/rotating_goal.gd, scripts/layout_math.gd), and ships with thorough tests. One acceptance-criteria item (UI rendering) is intentionally deferred to a follow-up; the manager exposes everything needed to wire that up later.

Change-by-Change Findings

scripts/milestone_manager.gd (new, +136)

• class_name MilestoneManager — matches the static-utility class style used elsewhere (rotating_goal.gd, layout_math.gd).
• MILESTONE_CATALOG — fixed 5-entry chain exactly matching the example sequence in #132 (build hut → stockpile food → build workshop → build garden → support third worker). Order, IDs, names, types, and targets are deterministic.
  • Minor deviation: the issue example says "Stockpile 5 food" but the PR uses amount: 10. The issue labels this as an Example chain, so this is a reasonable tuning choice, but worth flagging in the PR description for visibility.
• make_goal_state() — returns a JSON-serializable {milestone_id, completed_ids} dictionary; tests confirm the array is fresh (not a shared reference) and round-trips through JSON.stringify / JSON.parse_string.
• get_current_milestone(catalog, id) — deep-duplicates the entry so callers can mutate safely; returns {} for unknown IDs (defensive).
• evaluate_milestone(milestone, game_state) — pure-data, reads only builds, harvested, and workers keys (all existing game state). Returns {progress, total} for UI display.
  • BUILD: matches on kind + complete.
  • STOCKPILE: clamps mini(current, amount) — over-target does not regress.
  • WORKER: counts workers with break_ticks <= 0 (active). The break_ticks field exists in existing worker records per test_e2e.gd save-compatibility flow.
• is_milestone_complete() — simple wrapper, well-defined boundary.
• advance_to_next(completed_ids, current_id) — uses catalog index; the completed_ids parameter is unused, which is harmless but slightly noisy. Returns the current id at chain end or for unknown ids (defensive — matches test expectations).
• milestone_description() — convenient accessor for the event log / UI.

tests/test_milestone_goals.gd (new, +446)

• extends SceneTree — matches the pattern used by test_rotating_goal.gd and other headless tests, so it will plug into the existing test_runner.gd flow.
• 10 test functions cover: catalog structure/ordering, goal-state creation, current-milestone lookup, build/stockpile/worker evaluation (including clamping and break_ticks exclusion), completion boundary, chain advancement, description rendering, and a 7-case save/load round-trip including integration with a save_version: 2 save schema.
• Uses _assert / _assert_eq / _assert_str_eq helpers consistent with test_rotating_goal.gd. Exits with code 1 on failure so CI fails loudly.

Standards Compliance (AGENTS.md)

• Desktop companion feel / low-attention: milestone system is read-only with respect to game state and surfaces a single current goal — no urgent clicks, no modal replacement. Aligns with "the player plans and places; workers execute."
• Bottom-dock-first, no sidebar menus: no UI in this PR; the data layer is dock-agnostic.
• Conventions: pure-data class with static func mirrors rotating_goal.gd / layout_math.gd. Tab-indented GDScript, snake_case identifiers, MILESTONE_* constants for enum-like values.
• Validation: the new test file follows the headless SceneTree pattern (./.tools/Godot_v4.2.2-stable_linux.x86_64 --headless --path . --script res://tests/test_runner.gd) so it should slot into CI alongside the existing matrix.

Linked Issue Fit (#132)

• ✅ "Define a small fixed list of early milestones" — exactly 5 entries, hard-coded, deterministic order.
• ✅ "Milestones use existing game state only" — builds, harvested, workers only; no new state surface beyond the per-save milestone_id / completed_ids.
• ✅ "Completed milestones persist in save data" — make_goal_state() is a plain Dictionary; test_save_load_compatibility round-trips it through JSON.stringify / JSON.parse_string and embeds it inside a full save_version: 2 save.
• ⚠️ "UI shows the current milestone without replacing the ambient desktop-companion feel" — not addressed in this PR. The manager exposes get_current_milestone and evaluate_milestone so UI can read everything it needs, but no rendering/wiring in main.gd is included. This is a legitimate follow-up (git history shows a prior attempt at integrating MilestoneManager into main.gd was reverted in 7c2497c as "unrelated", suggesting scope discipline is intentional here). Worth calling out in the merge message so the UI piece doesn't get forgotten.
• ✅ "Tests cover completed/current milestone behavior and save/load compatibility" — comprehensive coverage in 10 functions + 7 sub-cases.
• ✅ "Avoid a full questline or story system" — no narrative, no branching, no rewards gating; just a fixed progression chain.

Evidence Provider Findings

• No evidence providers configured; nothing to report.

Tool Harness Findings

• Planning warning: Could not parse planning response as JSON — benign; no planned requests were made. No follow-up needed.

Unknowns / Needs Verification

• The PR doesn't touch main.gd, so the current milestone is not yet displayed anywhere in-game. Verify the follow-up UI PR is on the roadmap before closing #132.
• advance_to_next ignores its completed_ids argument. If a future contributor needs to validate against completed_ids (e.g. for reordering or branching), they'll need to extend it. Not a blocker.
• The class_name MilestoneManager introduces a global identifier; confirm no name clash with any future class in the main scene.

Sources

• PR #170 metadata, diff, and files (this corpus)
• Issue #132 body and acceptance criteria
• scripts/rotating_goal.gd and tests/test_rotating_goal.gd (existing comparable pattern, repo history)
• docs/DESIGN.md save schema (save_version 2, key inventory)
• tests/test_e2e.gd save-compatibility flow (worker field shape)
• AGENTS.md (project pillars, validation, contribution norms)
• CONTRIBUTING.md (test placement convention, persistence test requirement)

AI Automated Review

Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic)

PR Review: feat: add milestone goals for early progression (PR #170)

Recommendation: Approve

This PR cleanly implements a fixed early-game milestone system with solid test coverage. The implementation is well-structured, follows existing repository patterns, and introduces no regressions.

Change-by-Change Findings

scripts/milestone_manager.gd (+136 lines)

Design:

• All functions are static, making this a pure-data evaluation engine as documented. This is the correct architectural choice — the manager reads game state but doesn't hold mutable state.
• The MILESTONE_CATALOG is a const array with 5 fixed entries in a deterministic order: build_hut → stockpile_food → build_workshop → build_garden → support_third_worker. This matches the PR description.

Evaluation logic correctness:

• MILESTONE_TYPE_BUILD: Scans game_state.builds for a completed build of the specified build_kind. Returns {progress: 1, total: 1} when found, {0, 1} when not.
• MILESTONE_TYPE_STOCKPILE: Reads game_state.harvested[resource], uses mini(current, amount) for clamping at the target. Progress can go from 0 → 10.
• MILESTONE_TYPE_WORKER: Counts workers where break_ticks <= 0 (active workers). Clamps with mini(active, count). Target is 3 active workers for the final milestone.

Edge case handling:

• advance_to_next() returns the current ID when unknown or at the last milestone — defensive behavior is appropriate.
• get_current_milestone() returns {} for missing IDs.
• milestone_description() returns "" for empty descriptions (not the default string). This is intentional based on test expectations.

Save compatibility:

• Goal state schema is {milestone_id: String, completed_ids: Array} — both are JSON-native types. The tests verify round-trip through JSON.stringify/JSON.parse_string.

tests/test_milestone_goals.gd (+446 lines)

Coverage:

• test_catalog_exists: Validates 5 entries, all required fields (id, name, type, target, description), order, and all three milestone types present.
• test_make_goal_state: Verifies initial state starts at "build_hut" with empty completed_ids, and that the returned array is fresh (not shared).
• test_evaluate_build_milestone: Tests incomplete/complete builds, multiple build types, empty builds list.
• test_evaluate_stockpile_milestone: Tests 0, partial (4), exact (10), over-target (15, clamped to 10), and cross-resource interference.
• test_evaluate_worker_milestone: Tests 0/1/2/3 active workers, and that break_ticks > 0 workers are excluded.
• test_is_milestone_complete: Cross-milestone completion checks with a combined game state.
• test_advance_to_next: Full chain traversal from build_hut → support_third_worker, plus defensive cases.
• test_milestone_description: Verifies description strings, and that empty description returns empty string (not default).
• test_save_load_compatibility: 7 sub-tests covering fresh state, progress state, all-complete state, integration with full save schema, embedded round-trips, and post-load evaluation accuracy.

Test quality: The test is self-contained (extends SceneTree), runs headlessly, and uses quit(1) on failure so CI will catch regressions. Test names are descriptive and assertions include failure details.

Standards Compliance

Linked Issue Fit

No linked issue body was provided in the corpus. The PR body references #132 and the implementation matches the three bullet points in the PR description:

1. ✅ "Define fixed early-game milestone catalog" — 5 entries: build hut, stockpile food, build workshop, build garden, support third worker
2. ✅ "Add MilestoneManager with pure-data evaluation logic" — static functions that read game_state dictionaries only
3. ✅ "Add comprehensive unit tests" — 446 lines covering progression, save compatibility, and milestone description display

Unknowns / Needs Verification

None. The implementation is complete and testable via the repository's headless test runner. No blocking issues found.